-
Notifications
You must be signed in to change notification settings - Fork 3
Bump to compact 0.28.0 #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis pull request upgrades the Compact toolchain from version 0.26.0 to 0.28.0 and updates the compact-runtime dependency to 0.14.0-rc.0. The changes include refactoring the simulator core to use new runtime types (StateValue, ChargedState), replacing the CircuitContext structure to use currentQueryContext and costModel instead of originalState and transactionContext, migrating artifact imports from CommonJS to ES modules, and updating Compact contract files to language version 0.19.0 with named imports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/setup/action.yml (1)
42-46: Updatecompact-versionto an available release; version 0.28.0 does not exist.The latest released Compact compiler version is
0.26.0(October 8, 2025). Version0.28.0is not a published release and will cause the setup action to fail. Update to0.26.0or verify the correct version number.
🧹 Nitpick comments (2)
packages/cli/README.md (1)
238-244: Align remaining example versions for consistency.This output block shows 0.28.0, but earlier examples still reference 0.26.0 (e.g.,
+0.26.0and programmatic API snippets). Consider updating those to reduce confusion.packages/simulator/test/unit/core/StateManager.test.ts (1)
99-133: Avoid usingQueryContextas thecostModelplaceholder.
costModelis currently set tomodifiedTxCtx(aQueryContext). If the runtime expects a distinct cost model type, this can mask mismatches. Consider reusing the existing context’scostModel(or constructing a proper one) instead.Suggested tweak
const newCtx: CircuitContext<SimplePrivateState> = { currentQueryContext: modifiedTxCtx, currentPrivateState: initialPrivateState, currentZswapLocalState: zswapLocalState_1, - costModel: modifiedTxCtx, + costModel: ctx.costModel, };
0xisk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @andrew-fleming! Looking good! I left some comments.
packages/simulator/test/fixtures/sample-contracts/SampleZOwnable.compact
Outdated
Show resolved
Hide resolved
packages/simulator/test/fixtures/sample-contracts/Simple.compact
Outdated
Show resolved
Hide resolved
packages/simulator/test/fixtures/sample-contracts/Witness.compact
Outdated
Show resolved
Hide resolved
emnul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I just have an open question about ledger v7
emnul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick turnaround ftw! 🚀
0xisk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @andrew-fleming! Changes look good! Left some comments mainly regarding gasLimit is it excluded as we are dealing with the Simulator so the gasLimit here is useless?
Co-authored-by: 0xisk <0xisk@proton.me> Signed-off-by: Andrew Fleming <fleming.andrew@protonmail.com>
emnul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approve edits
0xisk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! thank you @andrew-fleming!
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #59.
Note that the root README badge still shows 0.26.0. Instead of changing it now, this PR proposes to wait until there's a release document for it to point to. This PR could change the badge's version number and keep the link if preferred (but it's easier to forget if the output if correct)
This PR also proposes not to update all the CLI tests that use 0.26.0 as an input value. It's simple to change if it's really desired
PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.